Filter upstream auth chain via callback hook#5725
Conversation
The embedded OAuth authorization server walks every configured upstream on every authorization, prompting for and storing tokens that a given authorization may not need. This widens the stored-token surface and forces needless upstream prompts. Add an optional UpstreamFilter hook (WithUpstreamFilter), consulted once in the callback handler after the first upstream resolves, that narrows the remaining chain to a subset of the configured upstreams: - The first upstream is always required and is never passed to nor removable by the filter. - The kept set is computed once and carried in PendingAuthorization (ChainUpstreams) so the filter is not re-run per leg. - nextMissingUpstream and the cross-leg identity check now operate on the effective chain rather than the raw config. - A filter error fails the authorization with a server error; it never silently falls back to walking every upstream. With no filter configured, behaviour is unchanged: the handler walks all configured upstreams as before. SingleLeg flows are unaffected and the upstream-token storage key contracts are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5725 +/- ##
==========================================
+ Coverage 70.65% 70.68% +0.03%
==========================================
Files 682 682
Lines 68854 68949 +95
==========================================
+ Hits 48651 48739 +88
- Misses 16657 16663 +6
- Partials 3546 3547 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tgrunnagle
left a comment
tgrunnagle
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: security, architecture-go, storage-persistence, test-coverage, general-quality
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | Legacy-pending recompute can use the wrong leg's request context for the filter | 9/10 | MEDIUM | Fix |
| 2 | Stored ChainUpstreams is trusted without validation against configured upstreams |
7/10 | MEDIUM | Fix |
| 3 | Identity-mismatch log loses structured fields after the verifyChainIdentity extraction |
7/10 | MEDIUM | Fix |
| 4 | Missing backward-compatibility test for legacy Redis pending records lacking chain_upstreams |
7/10 | MEDIUM | Fix |
Overall
This adds an optional UpstreamFilter hook, injected via WithUpstreamFilter, that narrows a multi-upstream OAuth authorization chain to a subset after the first upstream resolves. The implementation matches the linked issue's proposed shape closely, computes the effective chain once and carries it forward in PendingAuthorization.ChainUpstreams rather than re-invoking the filter per leg, and fails closed (server error, no fallback to a full walk) on filter error. computeChain's narrowing logic is small, pure, and has thorough table-driven coverage of every scenario called out in the linked issue. For existing no-filter deployments, behavior is unchanged end to end.
The findings below are all MEDIUM and none block merging. The most notable one (#1) is a narrow edge case in the "empty ChainUpstreams" recompute branch: it can't distinguish a genuine first leg from a non-first leg whose pending predates this field (a realistic rolling-deploy scenario for the Redis backend), so a context-sensitive filter could see the wrong leg's request context during that window. The other three are smaller and self-contained: a defense-in-depth gap where a chain loaded from storage isn't cross-checked against the configured upstream list, a logging regression where the identity-mismatch check lost its structured slog fields during extraction into verifyChainIdentity, and a test-coverage gap for the legacy-Redis-record backward-compatibility case (the repo already has a precedent for exactly this kind of test at redis_test.go:1109).
Documentation
UpstreamFilter's doc comment (pkg/authserver/server/handlers/handler.go) states it receives "the request context of the first leg's callback" — worth adding a note there about the rolling-upgrade edge case in finding #1, and mentioning that the context it receives already carries auth.WithPlatformUser, since that's the most concrete signal a real filter implementation would key off of.
Generated with Claude Code
Addresses #5725 review comments: - MEDIUM callback.go (3525733910): distinguish a true first leg (ResolvedUserID == "") from a subsequent leg; reject a subsequent-leg pending that lacks a computed chain instead of recomputing the filter against the wrong leg's request context. - MEDIUM callback.go (3525733913): validate a chain loaded from storage (non-empty, leads with the first configured upstream, names only configured upstreams) so a tampered PendingAuthorization row cannot shrink the chain and disable the cross-leg identity check. - MEDIUM callback.go (3525733916): restore discrete expected/got/provider slog fields on the identity-mismatch check after the verifyChainIdentity extraction, so log pipelines can still alert on it. - MEDIUM redis_test.go (3525733919): add a backward-compatibility test that loads a legacy pending JSON lacking chain_upstreams and asserts an empty ChainUpstreams. Also expands the UpstreamFilter doc comment (first-leg-context guarantee across rolling upgrades, WithPlatformUser in ctx) and updates existing chain tests to carry ChainUpstreams on subsequent legs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
size/L
tgrunnagle
requested review from
ChrisJBurns,
JAORMX,
jhrozek and
rdimitrov
as code owners
A useful upstream filter needs to key its narrowing decision on who authenticated — the subject and the authorization-relevant claims from the first upstream — not just on request context. Widen the hook to receive that identity explicitly. - FilterUpstreams now takes the canonical platform user ID and the resolved auth.PrincipalInfo (claim-mapped Subject, Name, Email, and the Claims map used by authz policies) alongside the configured upstreams. The principal is passed as an explicit parameter, not via context: the callback deliberately avoids placing a bearer-less Identity in ctx. - Capture the upstream claims that previously had nowhere to live: add Claims to upstream.Identity, populated from the validated ID token (OIDC) and the userinfo response (OAuth2). identityFromToken and synthetic OAuth2 resolve no structured claim set and leave it nil. - The callback builds the principal from the first leg's resolved identity and threads it through continueChainOrComplete -> resolveChain -> computeChain to the filter. Tests: an OIDC provider integration test (real provider + mock IdP) asserting ID-token claims are captured into the resolved Identity, and a callback integration test asserting the first upstream's subject and claims reach FilterUpstreams. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
size/L
The embedded auth server integration suite stopped at the first-leg redirect and never exercised the /oauth/callback or multi-upstream chain traversal. Add coverage that drives the callback end to end against the real embedded server and mock upstream IDPs: - Single upstream: authorize -> callback -> the chain completes at the sole upstream and issues the authorization code to the client. - Multi-upstream: the first callback continues the chain to the second upstream, and the second completes it and issues the code. Adds a WithUpstreams helper option, an OAuth2 upstream builder, and a Callback client method to support driving the flow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Went through this carefully, including tracing the storage / replay / cross-leg interleave angles. Overall it's solid: session isolation holds via the fresh per-/authorize sessionID, the filter can only narrow (never reorder or extend the chain), every failure path fails closed with token cleanup, and ChainUpstreams is cloned on all four storage crossings. Build and the authserver package tests pass locally.
Approving. A few non-blocking things:
- validateChain doesn't fully enforce what its doc comment claims — the one substantive item, left inline.
- NewHandler (pkg/authserver/server/handlers/handler.go) doesn't reject duplicate upstream names, even though NamedUpstream's doc says they must be unique. upstreamByName returns first-match and tokens key by name, and this PR now keys the chain itself by name, so the invariant is newly load-bearing. Worth a dedup check in the existing validation loop so it fails loudly at construction. (Pre-existing, so noting it here rather than inline since that block isn't in the diff.)
- Two doc clarifications inline (the filter Claims contract, and the verifyChainIdentity scope wording).
Addresses #5725 review comments: - MEDIUM handler.go (3528199173): validateChain now enforces an in-order, duplicate-free subsequence of the configured upstreams (config-cursor walk), so a tampered pending row cannot shrink/reorder the chain to skip legs (e.g. ["provider-1"] or ["provider-1","provider-1"]) and slip past the single-element identity-check no-op. Adds a TestValidateChain table covering wrong-first / unconfigured / out-of-order / duplicate. - MEDIUM handler.go (body): NewHandler rejects duplicate upstream names — the name invariant is now load-bearing (chain keyed by name; tokens and upstreamByName key by name). Adds a constructor test. - LOW handler.go (3528199199): document the Claims contract — values are untyped (assert defensively, never panic), nil is possible on a transient OIDC extraction failure (treat as fail-closed), and aud is the upstream's client_id, not this AS. - LOW callback.go (3528199204): reword verifyChainIdentity to match scope — it reconciles only the first leg; intermediate legs are intentionally not identity-checked. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Re: the duplicate upstream-name observation in your review body — addressed in 3dd8352. |
github-actions
Bot
added
size/XL
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
|
Added a Large PR Justification section to the description per the large-PR gate. In short: non-test code is ~371 lines (within the 400 guideline); the remaining ~849 lines are reviewer-requested test coverage for a cohesive feature. This review should auto-dismiss now that the section is present. |
github-actions
Bot
added
size/XL
github-actions
Bot
dismissed
their stale review
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
The canonical user is already carried as principal.PlatformUserID, so the separate platformUserID argument to FilterUpstreams was redundant. Remove it and have consumers read principal.PlatformUserID; update the doc, the stub filter, and the tests accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
size/XL
2 participants
Summary
The embedded OAuth authorization server walks every configured upstream provider
on every authorization. There is no way for a consumer to say that only a subset
of the configured upstreams is relevant for a given authorization, so the flow
prompts for upstreams the authorization does not need and stores upstream tokens
that are never used — widening the stored-token surface for no benefit.
This PR adds an optional filter hook, consulted once in the callback handler after
the first upstream resolves, that narrows the remaining chain to a subset of the
configured upstreams. The filter is given the identity established by the first
upstream — the canonical user plus the authorization-relevant claims — so it can
decide the subset based on who authenticated. The change is fully backward
compatible: with no filter configured, the handler walks all configured upstreams
exactly as before.
keyed on the authenticated principal — avoiding needless prompts and reducing
the stored-token surface.
UpstreamFilterinterface andWithUpstreamFilterconstructionoption. The filter receives the canonical platform user ID and the resolved
auth.PrincipalInfo(claim-mapped subject, name, email, and theClaimsmapused by authorization policies) alongside the non-first configured upstream
names. The effective chain is computed once on the first leg and carried in
PendingAuthorizationso the filter is not re-run per leg; subsequent legs reusethe carried chain after validating it against the configured upstreams. The
missing-leg walk and the cross-leg identity check operate on the effective chain
rather than the raw config. Upstream ID-token / userinfo claims — previously
discarded — are now captured into
upstream.Identityso the filter can use them.Closes #5724
Large PR Justification
This PR trips the >1000-line gate almost entirely because of test coverage, not
feature code:
+334 / -37) — within the 400-lineguideline. This is the cohesive
UpstreamFilterhook plus the review-drivenhardening (in-order chain validation, duplicate-upstream-name rejection at
construction, and the identity/claims plumbing through the callback).
added at reviewers' request across review rounds: an OIDC-provider integration
test for ID-token claim capture, callback/chain integration tests driven through
the real embedded auth server, a
validateChainsubsequence table test, aduplicate-name constructor test, and storage round-trip / legacy-record
backward-compatibility tests.
The feature and its tests are one logical change — cheaper to review and safer to
revert together than fragmented across PRs — and the change already carries a human
approval (@jhrozek).
Type of change
Test plan
task test)task test-e2e)task lint-fix)Unit and integration tests cover claim capture, the callback flow, chain
computation, identity propagation, and storage round-trips:
OIDCProviderImpl+ mock IdP): an ID tokencarrying
emailand a multi-valuedgroupsclaim is captured intoIdentity.Claims.the canonical platform user ID, reach
FilterUpstreamsduring callback handling.issues the code without prompting for the dropped leg.
pending authorization.
to a full walk).
is rejected and fails closed rather than recomputing against a later leg.
computeChaintable cases: no-filter full walk in order, single upstream skipsthe filter, subset kept with the first upstream always leading, empty keep set
leaves only the first upstream, filter return order ignored in favor of
configured order, unknown / first-upstream names in the keep set ignored, filter
error propagated, and the principal passed through to the filter verbatim.
nextMissingUpstreamrespects the chain subset rather than the raw config.ChainUpstreams(in-memory and Redis), plus abackward-compatibility test loading a legacy Redis pending without
chain_upstreams.Changes
pkg/authserver/server/handlers/handler.goUpstreamFilterinterface (takes the platform user ID +auth.PrincipalInfo+ configured upstreams),WithUpstreamFilteroption, andcomputeChain/resolveChain/validateChain;nextMissingUpstreamnow walks the effective chainpkg/authserver/server/handlers/callback.goverifyChainIdentity(with structured mismatch logging) gating on the effective chainpkg/authserver/storage/types.goChainUpstreamsfield toPendingAuthorizationpkg/authserver/storage/memory.go,redis.goChainUpstreamspkg/authserver/upstream/types.goClaimstoupstream.Identitypkg/authserver/upstream/oidc.go,oauth2.gopkg/authserver/**/*_test.goDoes this introduce a user-facing change?
No end-user-facing change to the
thvCLI. For consumers embedding the authserver, this adds a new optional construction option (
WithUpstreamFilter). It isopt-in and behavior is unchanged when unset.
Special notes for reviewers
upstreams[0]; it is not passed to the filter and cannot be removed by it.computeChainiterates theconfigured order and ignores any returned name that is not a non-first configured
upstream, so a filter cannot reorder the chain or inject unknown providers.
avoids placing a bearer-less
Identityinctx(no ToolHive bearer has beenminted yet). The filter receives the credential-free
auth.PrincipalInfo(Subject = claim-mapped upstream subject;
Claims= ID-token/userinfo claims,nil for
identityFromToken/ synthetic OAuth2) and must treat it as read-only.server error rather than reverting to walking every upstream.
ChainUpstreamsmust benon-empty, lead with the first configured upstream, and name only configured
upstreams — so a tampered pending row cannot shrink the chain to skip legs (which
would also disable the cross-leg identity check).
ChainUpstreamsfield existed is rejected (fail closed, forcing a freshauthorization) rather than recomputed against the wrong leg's context, preserving
the "filter runs once, on the first leg" guarantee.
SingleLegflows are unaffected and theupstream-token storage key contracts are unchanged.
Generated with Claude Code